Skip to content

🤖 feat: Agent Plugins install/update UX (managed installs, v1) - #3820

Open
ThomasK33 wants to merge 60 commits into
mainfrom
agent-plugin-install-ux
Open

🤖 feat: Agent Plugins install/update UX (managed installs, v1)#3820
ThomasK33 wants to merge 60 commits into
mainfrom
agent-plugin-install-ux

Conversation

@ThomasK33

@ThomasK33 ThomasK33 commented Aug 8, 2026

Copy link
Copy Markdown
Member

Summary

v1 of the Agent Plugins install/update UX ("Option B: managed installs"): paste a git URL or owner/repo[@ref] into Settings → Plugins, get a consent preview of everything the plugin contributes (manifest, every skill, every MCP server command line), and install into ~/.mux/plugins with provenance recorded in a managed-install registry. Update badge + manual update, uninstall with override pruning, all behind the existing agent-plugins experiment.

Background

PR #3815 shipped Agent Plugins 1.0.0 as discovery-only: users had to git clone into container dirs by hand, with no provenance, no update signal, no uninstall, and no list surface. The design doc (docs/research/agent-plugin-integration-options.md on branch research-agent-plugin-ux) compared five options; Thomas signed off on Option B (managed installs) with the §6 proposed decisions ratified.

Approved decisions implemented here

  1. Registry — a standalone ~/.mux/plugins.json owned by the install service (atomic, throwing writes; in-process serialized mutations). Lenient-on-read: invalid entries are dropped with a warning, and plugin names are pattern-validated so a malformed entry can never resolve a path outside the container. (Deviation from §6-Q2's letter, following its own contingency: Codex review demonstrated that .passthrough() only affects schema validation — older builds rebuild config.json from known fields on save, so a downgrade would drop an embedded registry section. Q2 priced exactly this: "Cost if wrong: a one-time migration to a separate file." A file older builds never rewrite is the only mechanism that actually survives downgrade round-trips, and owning the write path also makes registry-persistence failures observable for rollback.)
  2. Tracking semanticssource.ref is the tracking channel, lockedSha is what runs. No ref given ⇒ record the remote default branch + pin its current SHA. Tag/SHA refs are pinned (moved tags surface a tag moved warning badge). Nothing auto-applies, ever.
  3. Consent preview — temp shallow clone to ~/.mux/plugin-staging (never inside a discovery container), validated with the same validatePluginManifest + discovery code the runtime uses, listing manifest metadata, every skill name+description, and every MCP command line (rendered against the final install path, incl. PLUGIN_DATA expansion). Cancelling writes nothing — the preview is stateless; install re-fetches the exact consented SHA and fails loudly if the remote moved.
  4. Update — badge + manual only; checks run on Settings-section open and on the explicit button (git ls-remote vs lockedSha, no fetch, no timers). Applying = temp clone at the new SHA → re-validate → wholesale directory swap (rename-old → promote-new → delete-old, with rollback) → bump lockedSharecycle that plugin's running MCP servers via the new MCPServerManager.stopServersWithKeyPrefix (content can change behind an unchanged stdio command line, so the config-signature check cannot notice). Local edits to a managed plugin dir are discarded on update (documented).
  5. Uninstall — deletes plugin dir + registry entry + prunes that plugin's plugin:<instanceId>:* keys from every local workspace's MCP overrides (reinstall re-attaches the same instanceId, so stale overrides would silently re-enable servers). ~/.mux/plugin-data/<instanceId> is preserved behind an "also delete stored plugin data" checkbox, unchecked by default.
  6. Scope — global-only; the installer never writes into a project checkout.
  7. Human-only surfaces — Settings section + palette commands (Settings: Plugins, Install Agent Plugin…, Check for Plugin Updates, Update All Plugins — keyboard rule). No agent-facing installer tool.
  8. Name collisions — existing registry entry or target dir ⇒ clear error; the installer never overwrites.
  9. Subpath grammar, not subpath installsowner/repo/sub/path[@ref] parses and the subpath field is persisted in the source descriptor, but installs reject with "monorepo subpath installs land in v2". Claude Code plugin/marketplace repos fail with a clear message naming the limitation (source stays a discriminated union so an import adapter is additive).

Implementation

  • Step 1 — registry schema: src/common/config/schemas/agentPluginInstalls.ts (entry + tagged-union source + plugins.json file schema); name grammar shared with the manifest validator via src/common/utils/agentPluginName.ts.
  • Step 2 — service + oRPC: discoverAgentPluginAt (public single-root wrapper over the existing per-entry discovery, so staged clones get the exact runtime validation); normalizeRepoUrlForClone extracted to src/node/utils/gitUrls.ts (shared with the project clone flow); sourceInput.ts grammar; AgentPluginInstallService (preview/install/list/uninstall/checkUpdates/update, mutations serialized on an internal queue, staging under ~/.mux/plugin-staging with stale-dir reclamation, GIT_TERMINAL_PROMPT=0 + SSH BatchMode so private repos without auth fail fast instead of hanging); plugins.* oRPC namespace returning Result values; MCPServerManager.stopServersWithKeyPrefix recycle hook. Backend gating mirrors the MCP provider: the service is constructed with isEnabled: () => experimentsService.isExperimentEnabled(AGENT_PLUGINS).
  • Step 3 — UI: PluginsSettingsSection (list with unmanaged/missing/update available/tag moved/pinned badges, two-phase add flow, inline uninstall confirm), experiment-gated section registration + redirect + palette entry.
  • Step 4/5 — docs, stories, tests: docs additions in docs/config/mcp-servers.mdx + docs/agents/agent-skills.mdx; Storybook stories with play assertions (consent preview, update states, unchecked-by-default checkbox); unit tests for the input grammar, registry round-trip/self-heal, and the full service lifecycle against real local git remotes (hermetic — local-path remotes exercise the same clone/ls-remote plumbing).

Validation

  • make static-check green (typecheck, ESLint, prettier, docs links); targeted suites: 393 tests across the touched areas (agentPlugins, config, schemas, SettingsPage, palette sources, MCPServerManager, oRPC router, projectService) all pass; test-storybook passes for the new stories.
  • Live dogfooding in a make dev-server-sandbox instance (screenshots in the workspace transcript): enabled the experiment via Settings → Experiments (Plugins section appeared immediately), installed a local fixture repo through the full preview → consent → install flow, verified the on-disk registry entry + plugin tree (no .git), advanced the fixture remote → update available badge appeared on "Check for updates" → Update bumped lockedSha/version/updatedAt, uninstall (checkbox unchecked) removed dir + registry but preserved plugin-data, and the reinstalled plugin's MCP server surfaced in Settings → MCP as plugin · … default-disabled/read-only.

Risks

  • Config surface: none — the registry is a standalone ~/.mux/plugins.json; config.json load/save is untouched. Malformed registry entries degrade to "unmanaged dir" rather than errors; downgrade-safe because older builds never touch the file.
  • MCP recycle: stopServersWithKeyPrefix only stops matching workspaces' server sets; they restart lazily on next use, same as the idle-timeout path. No behavior change for non-plugin servers.
  • Everything is experiment-gated: with agent-plugins off, the service throws, the section/palette entry hide, and no new code paths run.

Judgement calls

  • install re-fetches the exact consented SHA (direct SHA fetch, falling back to branch clone + HEAD verification) rather than keeping the preview clone on disk between preview and confirm — a stateless preview means cancel/crash cannot leave partial state, at the cost of a second shallow clone on confirm.
  • The installed tree drops .git (plain content snapshot): the registry holds all provenance, updates replace the dir wholesale, and a live checkout would only invite in-place edits that updates discard.
  • Update refuses upstream renames (plugin.json#name changed): container-entry names are identity (instanceId → PLUGIN_DATA, workspace overrides), so renames require uninstall/reinstall.
  • Uninstall stops that plugin's running MCP servers before deleting the tree, mirroring the update-recycle rationale.
  • Update All Plugins applies only update-available entries; moved tags stay per-plugin manual (a mutated tag deserves the section's warning, not a bulk apply).

Deferred (per §5/§6 of the design)

  • v2: monorepo subpath installs (sparse checkout; grammar + schema already in place), content-addressed store + symlinked container entries, dev-mode/local-path installs, unmanaged-dir adoption ("convert to managed"), Pin row action, bun run debug plugin … CLI + /plugin slash command.
  • v3: repo-declared prompt-on-trust team plugins, archive+sha256 / seed dirs for air-gap, restore-from-lock, catalogs/marketplace (Claude marketplace import adapter only on demonstrated demand).
  • Explicit non-goals: background/auto-update (per-entry autoUpdate boolean reserved in the schema, unused), agent-facing install tool, Claude Code marketplace compatibility.

Post-review hardening (Codex rounds 1–20)

20 review rounds of fixes folded into this diff

Highlights beyond the original plan (full round-by-round history in the PR review threads):

  • Registry durability: standalone plugins.json with lossless raw-document writes (unknown envelope/entry/tombstone fields from newer builds survive rewrites), strict-mode reads for mutations vs lenient reads for list, raw-entry collision checks, non-ENOENT read errors refuse mutations.
  • Uninstall override pruning: pre-commit workspace enumeration, persisted pendingOverridePrunes tombstones (pessimistic commit, retry on list, reconciliation against deleted workspaces, reinstall gate) so a temporarily unreachable checkout can never let a reinstall silently re-enable a pruned server.
  • Workspace MCP overrides optimistic concurrency: workspace.mcp.get returns { overrides, revision }; set requires expectedRevision and rejects stale dialog snapshots (serialized check-and-set), and the uninstaller's prune retries on conflict — a stale open dialog can no longer resurrect pruned plugin: keys.
  • MCP recycle vs in-flight startups: monotonic prefix-invalidation epochs; closeInvalidatedInstancesThenPublish re-scans until the invalidation clock is stable and publishes synchronously in the same continuation, closing the microtask window where a mid-startup plugin swap could publish (and keep alive) a server from a deleted tree. Regression test interleaves the swap into the exact yield window.
  • Consent preview parity: symlinked skill dirs are disclosed (with containment warnings for escaping symlinks), matching runtime discovery.
  • Mobile: break-all on plugin name/location/source lines + pinned 390px story with a max-length-name overflow assertion.

Two P2 follow-ups are documented (not in this PR) in this comment: stale workspace-switch modal loads, and prefix-stop retry-marker publish ordering.

Rebase + experimental label

  • Rebased onto main (squashed to one feature commit): merged this branch's install contract with main's independently-landed agentPlugins.ts oRPC schemas (slash commands/composition inspector), re-ported the stable-clock publication onto the MCP SDK v2 mcpServerManager, and adopted main's SSH→HTTPS clone fallback into the extracted gitUrls.ts (installer records the primary URL only, documented).
  • The Plugins section is now marked experimental the same way as Backup: FlaskConical nav icon + in-section warning banner.
  • Re-dogfooded the full lifecycle post-rebase in a fresh dev-server-sandbox (install → consent preview incl. a deliberately-invalid mcp.json diagnostic → update badge → atomic update → read-only MCP row → uninstall with data-preservation default); screenshots in the workspace transcript.

Generated with mux • Model: anthropic:claude-fable-5 • Thinking: xhigh • Cost: $327.63

@mintlify

mintlify Bot commented Aug 8, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
Mux 🟢 Ready View Preview Aug 8, 2026, 6:37 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3b9d7245ce

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentPlugins/installService.ts Outdated
Comment thread src/common/config/schemas/appConfigOnDisk.ts Outdated
Comment thread src/node/services/agentPlugins/installService.ts Outdated
Comment thread src/node/services/agentPlugins/installService.ts Outdated
Comment thread src/browser/features/Settings/Sections/PluginsSettingsSection.tsx
@ThomasK33

Copy link
Copy Markdown
Member Author

Addressed all five Codex findings in b9c9a10:

  • Traversal names (P1) — the plugin-name grammar (§5 pattern, now shared via src/common/utils/agentPluginName.ts) is enforced in the registry entry schema and asserted in targetPathFor before any filesystem mutation; entries named ./../a/../b are dropped on read and can never resolve outside the container. Test: "registry survives config.json rewrites and drops traversal names on read".
  • Downgrade preservation (P1) — correct: .passthrough() only affects schema validation; older builds rebuild config.json from known fields on save. Followed §6-Q2's own contingency ("migration to a separate file"): the registry now lives in a standalone ~/.mux/plugins.json that older builds never rewrite. Test: registry survives editConfig config.json rewrites.
  • Registry write observability (P2) — solved by the same move: the service owns the file and its atomic write throws, so install rolls back the promoted dir ("Failed to persist the plugin registry"), uninstall writes the registry before deleting the tree, and a failed update write keeps the stale lockedSha (badge stays, retry self-heals). Test: "install rolls back the promoted dir when the registry write fails".
  • Fallback clone into non-empty dir (P1) — the staging dir is reset before the branch-clone fallback. Test: "falls back to a branch clone when the remote refuses direct SHA fetches" (file:// remote with uploadpack.allowAnySHA1InWant=false).
  • Keyboard rule (P1) — added palette commands: Install Agent Plugin… (opens Settings → Plugins with the add form expanded), Check for Plugin Updates (toast + navigates when updates exist), Update All Plugins (applies update-available; moved tags intentionally stay per-plugin manual since mutated tags deserve the section's warning). Uninstall/per-plugin update remain reachable via standard focus navigation within the section.

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b9c9a1062a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/browser/features/Settings/Sections/PluginsSettingsSection.tsx Outdated
Comment thread src/node/services/agentPlugins/installService.ts Outdated
Comment thread src/browser/utils/commands/sources.ts Outdated
Comment thread src/browser/utils/commands/sources.ts Outdated
@ThomasK33

Copy link
Copy Markdown
Member Author

Addressed all four round-2 findings in edcdfa0:

  • Registry restore on removal failure — uninstall now stages the tree out of the container (rename into the staging root) before the registry write: a locked/undeletable tree fails the rename with the install fully intact, and a failed registry write renames the tree back. Deleting the staged tree is best-effort (stale-dir reclamation covers leftovers). Test: "uninstall restores the registry entry when the tree cannot be staged out" (read-only container forces the rename failure, then the retry succeeds).
  • Add panel with section already mounted — the intent module now supports subscription; the mounted section subscribes and expands the add panel immediately, while the useState initializer still covers the palette → fresh-mount path.
  • Mutation errors clobbered by refresh — update/uninstall re-assert the operation error after the refresh (whose success path clears error state); a failed uninstall also keeps the confirmation open instead of dismissing it.
  • Per-plugin check errors — both Check for Plugin Updates and Update All Plugins now distinguish status: "error" entries: unreachable remotes surface as "Update check failed for …" (with navigation to the section) instead of masquerading as "All plugins are up to date."

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: edcdfa05fe

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/browser/features/Settings/Sections/PluginsSettingsSection.tsx
Comment thread src/browser/utils/commands/sources.ts
Comment thread src/node/services/agentPlugins/sourceInput.ts
@ThomasK33

Copy link
Copy Markdown
Member Author

Addressed all three round-3 findings:

  • Keyboard path for uninstall — new Uninstall Agent Plugin… palette command using the palette's select prompt (async getOptions over agentPlugins.list(), managed entries only). Submission publishes a confirm-uninstall intent and opens the section, landing the user in the existing confirmation flow with the plugin-data checkbox — the palette never deletes directly.
  • Mounted-section staleness after bulk updates — the intent module is now a typed bus (open-add-panel / confirm-uninstall / refresh); Update All Plugins publishes refresh after its mutations, so a mounted section re-queries list + update checks instead of showing stale versions/badges. Unmounted sections still consume the buffered intent on mount.
  • Tilde expansion~/~/… sources resolve against os.homedir() before git sees them (git is spawned via execFile, no shell). Grammar test added.

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a6503d2fe4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentPlugins/installService.ts Outdated
Comment thread src/node/services/agentPlugins/installService.ts
Comment thread src/browser/utils/commands/sources.ts Outdated
Comment thread src/browser/features/Settings/Sections/PluginsSettingsSection.tsx
Comment thread src/node/services/agentPlugins/sourceInput.ts Outdated
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bc68e771d7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/mcpServerManager.ts Outdated
Comment thread src/node/services/agentPlugins/installService.ts
Comment thread src/node/services/agentPlugins/installService.ts
Comment thread src/browser/utils/commands/sources.ts
Comment thread src/node/services/agentPlugins/installService.ts Outdated
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5a2bd105f4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/mcpServerManager.ts
Comment thread src/node/services/mcpServerManager.ts Outdated
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0443e1af3e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentPlugins/installService.ts
Comment thread src/browser/features/Settings/Sections/PluginsSettingsSection.tsx Outdated
Comment thread src/node/services/agentPlugins/installService.ts Outdated
Comment thread src/browser/features/Settings/Sections/PluginsSettingsSection.tsx
Comment thread src/node/services/agentPlugins/installService.ts Outdated
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dc141c4c7e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentPlugins/installService.ts Outdated
Comment thread src/node/services/agentPlugins/installService.ts Outdated
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b6daf6aec0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentPlugins/installService.ts Outdated
Comment thread src/node/services/agentPlugins/installService.ts
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f1fd47e8cd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentPlugins/installService.ts Outdated
Comment thread src/node/services/agentPlugins/installService.ts
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b6965bc298

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/browser/utils/commands/sources.ts Outdated
Comment thread src/browser/utils/commands/sources.ts Outdated
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e576538423

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentPlugins/installService.ts Outdated
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Breezy!

Reviewed commit: 6d12bf5c2c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

…er snapshot cannot re-enable a sibling-reinstalled plugin server
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

Round 66 fix (9e113da):

  • Cold first-serve disk-authoritative overrides: ensureWorkspaceServers now loads a workspace's overrides from disk (loadFirstServeWorkspaceOverrides) on the workspace's FIRST serve on this manager, instead of trusting the caller's snapshot — which may have been read before a sibling process's uninstall + same-name reinstall pruned the enable on disk. This covers both the cold-manager case (first token observation records the already-advanced epoch with nothing to retire) and the never-served-workspace case (sweeps refresh only workspaces with recorded options). When disk cannot be read, plugin keys are scrubbed from the caller snapshot instead; off-host workspaces keep the existing skip. Added the cold-manager regression test: a stale caller enable against a pruned disk file starts nothing.

Thread resolved; make static-check and the mcpServerManager suite (124/124, incl. the new regression) pass locally.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9e113da27c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/mcpServerManager.ts Outdated
Comment thread src/node/services/agentPlugins/installService.ts
…t-serve disk read, cap hooks.js source size in discovery)
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

Round 67 fixes (6a708bf):

  • First-serve cache recheck: ensureWorkspaceServers now rechecks latestWorkspaceOverrides AFTER the first-serve disk read resolves. A settings save completing while the read was in flight publishes newer state into the cache; the continuation now prefers that over the read's older result, so a just-disabled server can no longer be exposed for the current send (the save's repair path only patches recorded options, which don't exist yet on a first serve). Added a deterministic regression test that parks the serve on the disk read, applies a save, then resolves the read with the pre-save state.
  • hooks.js source size cap: component resolution in discovery now excludes a hooks.js larger than 1 MiB (error diagnostic, hook component only — §11.3 isolation preserved). Discovery is the shared chokepoint for the consent preview, the update capability comparison, and runtime loading, so an oversized hook is identically absent from all three: it can never be accepted at install, and hookService never reads/hashes/evaluates it. Added a discovery regression test.

Both threads resolved; make static-check plus the agentPlugins (216) and mcpServerManager/discovery (151) suites pass locally.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Security Review

Here are some automated security review suggestions for this pull request.

Reviewed commit: 6a708bf773

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

Comment thread src/node/services/agentPlugins/installService.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6a708bf773

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentPlugins/discovery.ts
Comment thread src/node/services/agentPlugins/installService.ts
Comment thread src/node/services/agentPlugins/installService.ts Outdated
@ThomasK33

ThomasK33 commented Aug 22, 2026

Copy link
Copy Markdown
Member Author

@codex review

Round 68 fix (908adb5):

  • Nested plugin workflow consent alignment: parsePluginWorkflowScriptPath now rejects nested relative paths after normalization, so the executable set exactly matches the consented surface — the install preview, update capability comparison, and runtime lister all name TOP-LEVEL workflows/*.js only, and an upstream can no longer add workflows/private/hidden.js that consent never names yet workflow_run would execute. Top-level symlinks remain listed/consented under their top-level names and stay containment-checked. Added a resolver regression test for the nested-path rejection.

Thread resolved; make static-check and the workflowScriptResolver suite (17/17) pass locally.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 908adb52ab

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentPlugins/installService.ts
Comment thread src/node/utils/main/crossProcessLock.ts Outdated
Comment thread src/node/services/agentPlugins/installService.ts
Comment thread src/node/services/workspaceService.ts
Comment thread src/node/services/agentPlugins/installService.ts
…ead, release failed update trash for reclamation, fail update on epoch publication failure)
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

Round 69 fixes (4b6d9be):

  • Hook size cap enforced at the read: new readHookSourceCapped reads hooks.js through one file handle with a same-handle fstat size check, closing the update-sized TOCTOU window between discovery's stat-based cap and hookService's consuming read; reading exactly the fstat-reported byte count through the handle also bounds the read if the file grows mid-read. Added a helper regression test (oversized refused, normal source round-trips).
  • Failed update trash released for reclamation: when deleting the replaced tree fails after the journal is consumed, the catch now removes trashDir from activeStagingPaths — the transaction no longer owns it, and pinning it made every later purgeStaleStaging skip the very dir the catch defers to reclamation. Regression test asserts no trash- entry stays pinned after an update whose deletion fails.
  • Epoch publication failure fails the update: on the missing-tree path (no journal), the explicit mutation-epoch bump is the only cross-process publication of the swap; its failure now fails the update with a retry-able error instead of committing success, so a sibling still serving the removed tree's MCP server cannot miss the invalidation. The registry keeps the old lockedSha (update badge stays visible) and the retry runs the journaled swap path, whose journal lifecycle republishes the epoch. Regression test covers fail-then-retry self-healing.

All three threads resolved; make static-check plus the agentPlugins suites (hookService 14, installService 100, directory 219 pre-lint-fix) pass locally.

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4b6d9be3dd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentPlugins/hookService.ts
…kills, release unlink retry, update recovery provenance reconciliation, CLI registration sanitization, fresh-install override sweep)
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

Round 70 fixes (35b8e89):

  • Pre-read size checks: collectAgentFiles and collectSkills now check the stat size against the runtime cap (MAX_FILE_SIZE) BEFORE reading/decoding, mirroring runtime discovery's ordering, so an untrusted repo cannot stall the main process with one giant markdown file during Preview/Update.
  • Release unlink retry: release() now retries a transiently failing lock-file unlink within its bounded attempt loop instead of swallowing the error — a leftover holder record with renewal stopped read as a live owner for up to the stale ceiling. Regression test injects two EBUSY failures and asserts the lock is gone and reacquirable.
  • Update recovery provenance: the update journal now records newSha; nonce-match recovery reconciles the registry (lockedSha + updatedAt + version/description re-read from the promoted tree's plugin.json, raw-patch rules preserved) BEFORE consuming the journal, distinguishing crash-before-registry-write from the cleanup-only retry (registry already records newSha). Regression test simulates the crash (journal delete + registry write both fail) and asserts recovery commits the new SHA/version.
  • CLI registration sanitization: AgentSession.ensureMetadata (xum run/workflow in an unregistered directory) now routes through WorkspaceService.sanitizeCliRegisteredWorkspace between the config write and the announcement — same pending-set bookkeeping, host-local gating, and rollback-on-failure as create/fork/task flows — so a preserved checkout's stale plugin: enable can't activate a reinstalled server on the first CLI send.
  • Fresh-install override sweep: install() now runs assertNoResidualInstanceState — stop the prefix's cached servers, live-enumerate workspaces, and prune the canonical prefix — closing the manually-deleted-unmanaged-plugin gap the tombstone gate (managed uninstalls only) never covered. Fails closed on enumeration/prune failure. Regression tests cover the sweep and the fail-closed refusal.

All five threads resolved; make static-check plus the agentPlugins + crossProcessLock suites (238 tests, incl. 4 new) pass locally; 7 existing sequence-sensitive tests updated for the new install hygiene sweep.

readHookSourceCapped's open follows symlinks: a managed update replacing a
consented regular hooks.js with an absolute link outside the plugin root
(allowed by staged validation, read as a capability removal by discovery)
could have the stale canonical pathname follow the new link and evaluate an
outside file as hook code. Require the opened object to be the regular file
a non-following lstat sees at the path (dev/ino identity, bigint stats).
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

Round 71 fix (aac2280):

  • Hook read symlink follow (P2): readHookSourceCapped now requires the opened object to be the very regular file a non-following lstat sees at the path (bigint dev/ino identity match). A replacement symlink promoted between discovery and the consuming open — previously followed to a file outside the plugin root — is refused, as is any concurrent file replacement (over-blocking is safe; the next discovery re-measures). Added a symlinked-hooks.js regression test.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

const source = await readFileString(localRuntime, resolvedPath);

P2 Badge Pin plugin workflow reads to the contained file

When a top-level plugin workflow is resolved concurrently with a managed update, containment can validate the old regular workflows/foo.js, after which promotion replaces that pathname before this separate read. The replacement may be an escaping absolute symlink that stable-tree discovery would reject, but readFileString follows it anyway; a link to an outside named pipe hangs the workflow indefinitely, while a link to another JavaScript file executes source outside the consented plugin tree. Read through a non-following handle and verify the same opened object that passed containment and the size check.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/agentSession.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Security Review

Here are some automated security review suggestions for this pull request.

Reviewed commit: aac2280844

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

Comment thread src/node/services/agentPlugins/hookService.ts
…x round 72 P2)

xum run and xum workflow construct AgentSession directly, bypassing
WorkspaceService.createSession, so the optional
sanitizeCliWorkspaceRegistration callback was silently skipped and a
preserved checkout could carry a stale plugin: MCP enable into a
same-name reinstall on the first CLI send. Pass the sanitizer at both
headless entry points.
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

Round 72 fix (3bca011):

  • Direct CLI session sanitization (P2): xum run and xum workflow construct AgentSession directly (bypassing WorkspaceService.createSession), so the optional sanitizeCliWorkspaceRegistration callback was silently skipped for headless registrations. Both entry points now pass the sanitizer wired to WorkspaceService.sanitizeCliRegisteredWorkspace, so a preserved checkout cannot carry a stale plugin: enable into a same-name reinstall on its first CLI send.

…me (Codex round 73 P2)

The leaf lstat identity check cannot catch a replacement symlink at an
ancestor component (lib/hooks.js where lib becomes a link to an outside
dir): lstat follows ancestor links, so the outside file reports as
regular with matching dev/ino. readHookSourceCapped now also rechecks
full symlink-resolved containment against the plugin root after the
open; a promotion is a single swap, so a link the open followed is
still present at the recheck.
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

Round 73 fix (d59e33e):

  • Ancestor-symlink escape at hook read (P2): the leaf lstat identity check couldn't catch a replacement symlink at an ancestor component (lib/hooks.js where lib becomes a link to an outside dir) — lstat follows ancestor links and reports the outside file as regular with matching dev/ino. readHookSourceCapped now takes the plugin root and rechecks fully-resolved containment (ensurePathContained) after the open, in addition to the leaf identity check. A promotion is a single swap, so any link the open followed is still present at the recheck and gets rejected. Added an ancestor-symlink regression test.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3bca011dd8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/cli/run.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d59e33e4b2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

`mcp.json is too large (${stat.size} bytes; max ${MAX_PLUGIN_MANIFEST_BYTES})`
);
}
raw = JSON.parse(await fsPromises.readFile(plugin.mcpConfigPath, "utf8")) as unknown;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Revalidate the MCP config handle before parsing

When an enabled managed plugin server is updated so its new mcp.json is an absolute symlink to an existing file outside the plugin root, staged discovery treats MCP as removed and permits the update; if runtime discovery captured the old canonical path before promotion, this later readFile follows that path into the replacement tree and parses the outside configuration. An attacker-controlled file can preserve the enabled server name while changing it to a bare command such as sh, which is spawned before the mutation-epoch post-check can retire the stale result, yielding command execution during the race. Read through a bounded handle and revalidate containment plus file identity before using the configuration.

AGENTS.md reference: AGENTS.md:L136-L136

Useful? React with 👍 / 👎.

// freshly started server would otherwise publish validly and keep
// running from the removed tree. This runs BEFORE override pruning so
// pruning problems cannot skip the correctness-critical invalidation.
await this.deps.mcpServerManager?.stopServersWithKeyPrefix(serverKeyPrefix);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Retire registered hooks when uninstall commits

When a workspace has an active agent stream while its managed plugin is uninstalled, this post-removal invalidation closes only MCP servers; the plugin's already-registered AgentPluginHookService middleware remains live until another send calls ensureWorkspaceHooks or the workspace is disposed. The current stream can therefore continue exposing later tool arguments/results to, and accepting rewrites or denials from, a hook after uninstall has reported success. Explicitly invalidate affected workspace hook registrations at commit time, including in sibling processes via the mutation epoch.

AGENTS.md reference: AGENTS.md:L150-L150

Useful? React with 👍 / 👎.

…(Codex round 74 P2)

Both CLI entry points omit workspaceMcpOverridesService, and only the
desktop ServiceContainer called setWorkspaceMcpOverridesService, so the
CLI registration sanitizer early-returned on the undefined service and a
stale checkout-local plugin: enable survived headless registration.
createCoreServices now default-constructs the overrides service, passes
it to AIService and the plugin-invalidation override reader, and wires
WorkspaceService pruning for every process that can register
workspaces; the desktop's now-redundant explicit set call is removed.
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

Round 74 fix (f98dd52):

  • CLI override sanitizer actually wired (P2): both CLI createCoreServices() calls omit workspaceMcpOverridesService and only the desktop ServiceContainer called setWorkspaceMcpOverridesService, so sanitizeStalePluginOverridesForNewWorkspace() early-returned on the undefined service in headless registration. createCoreServices now default-constructs the overrides service when the caller doesn't pass one and wires it into AIService, the plugin-invalidation override reader, and WorkspaceService pruning — every process that can register workspaces sanitizes. The desktop's now-redundant explicit set call was removed (it passes the same instance via opts).

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f98dd52dc5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

// override reads AND registration-time plugin-override sanitization must
// work in every process that can register workspaces, not just desktop.
const workspaceMcpOverridesService =
opts.workspaceMcpOverridesService ?? new WorkspaceMcpOverridesService(config);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve live overrides in ephemeral CLI sessions

When xum run or xum workflow is launched inside a checkout already registered by the desktop app, the caller's config points at a temporary directory and explicitly contains no persistent workspace records (src/cli/run.ts:435-470 and src/cli/workflow.ts:204-223). Constructing the override service from that config means the registration sanitizer sees only the newly added ephemeral workspace, cannot detect the persistent live sibling for the same checkout, and prunes every canonical plugin entry from the shared .xum/mcp.local.jsonc; merely invoking the CLI therefore disables the desktop workspace's configured plugin servers and loses its allowlists. The sanitizer needs access to persistent sibling metadata, or a path-based pruning API that separates the ephemeral workspace record from the metadata used for the live-sibling check.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant